Skip to content

Conversation

@andysalerno
Copy link
Contributor

@andysalerno andysalerno commented May 14, 2025

I have encountered a problem very similar to #13471 .

That issue was fixed by allowing null or empty value for "content".

But my scenario (using pydantic-ai as a client) has requests where "content" is completely absent, in scenarios where there is a tool request.

If you look at the code for "at()" in json.hpp, you'll see it throws if the property is missing entirely.

This PR change fixed the issue for me locally.

Edit - here is an example of a message within a chat completions requests that has no content:

    {
      "role": "assistant",
      "tool_calls": [
        {
          "id": "qdAlNVrVB7fYP10SidYiSKx9MMbJCIb7",
          "type": "function",
          "function": {
            "name": "search_web",
            "arguments": "{\"query\":\"What is llama.cpp?\"}"
          }
        }
      ]
    }

looks related to #13516

@CISC
Copy link
Collaborator

CISC commented May 14, 2025

Looks good, but can you also check if google/minja#63 needs to be applied to fix this completely?

Copy link
Collaborator

@ngxson ngxson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should check if message contains either content OR tool_calls

@pwilkin
Copy link
Collaborator

pwilkin commented May 14, 2025

@ngxson I think it's even more finegrained: only assistant role messages can contain either content or tool_calls. System, user and tool role messages have to contain content.

@pwilkin
Copy link
Collaborator

pwilkin commented May 14, 2025

Per @ngxson comment ahove, added a pull request that checks for all the missing elements and produces meaningful error messages.

@pwilkin
Copy link
Collaborator

pwilkin commented May 14, 2025

Also @CISC , tested on my case and tool calling seems to work properly now, doesn't seem that integrating the minja fix is required.

@CISC
Copy link
Collaborator

CISC commented May 14, 2025

Superceded by #13540

@CISC CISC closed this May 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants